-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: support send flashback cluster RPC #37659
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/run-check-issue-triage-complete |
Updated, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// A hack way to make global variables are synchronized to all tidb. | ||
// TiKV will block read/write request during flashback cluster, | ||
// So it's not very dangerous when sync failed. | ||
time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why here is 1s
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see the comments, just let most TiDB synced this global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1s
just a magic number. I found the global variable will synced by ETCD, so the time cost will be very small
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
endKey = rangeEndKey | ||
} | ||
|
||
req := tikvrpc.NewRequest(tikvrpc.CmdFlashbackToVersion, &kvrpcpb.FlashbackToVersionRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set MaxExecutionDurationMs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be set to timeout
in client-go.
https://github.com/tikv/client-go/blob/426055b5755b4a64df7b92b278aee6e65479d53c/internal/locate/region_request.go#L962-L965
job.State = model.JobStateCancelled | ||
return ver, errors.Trace(err) | ||
} | ||
job.Args[gcEnabledArgsOffset] = &gcEnableValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to do it here? Or maybe we can do it in StateWriteOnly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should store it in the first stage, like onRecoverTable
func.
Lines 430 to 453 in e75a079
// 1. Check GC enable status, to decided whether enable GC after recover table. | |
// a. Why not disable GC before put the job to DDL job queue? | |
// Think about concurrency problem. If a recover job-1 is doing and already disabled GC, | |
// then, another recover table job-2 check GC enable will get disable before into the job queue. | |
// then, after recover table job-2 finished, the GC will be disabled. | |
// b. Why split into 2 steps? 1 step also can finish this job: check GC -> disable GC -> recover table -> finish job. | |
// What if the transaction commit failed? then, the job will retry, but the GC already disabled when first running. | |
// So, after this job retry succeed, the GC will be disabled. | |
// 2. Do recover table job. | |
// a. Check whether GC enabled, if enabled, disable GC first. | |
// b. Check GC safe point. If drop table time if after safe point time, then can do recover. | |
// otherwise, can't recover table, because the records of the table may already delete by gc. | |
// c. Remove GC task of the table from gc_delete_range table. | |
// d. Create table and rebase table auto ID. | |
// e. Finish. | |
switch tblInfo.State { | |
case model.StateNone: | |
// none -> write only | |
// check GC enable and update flag. | |
if gcEnable { | |
job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagEnableGC | |
} else { | |
job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagDisableGC | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but I think putting it to model.StateWriteOnly
is also OK. But it should be a little bit easier to understand here
assert.Equal(t, tk.ResultSetToResult(rs, "").Rows()[0][1], variable.On) | ||
rs, err = tk.Exec("show variables like 'tidb_gc_enable'") | ||
assert.NoError(t, err) | ||
assert.Equal(t, tk.ResultSetToResult(rs, "").Rows()[0][1], variable.Off) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about user do set global tidb_gc_enable = on"
in this state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an illegal operation, we will add it into doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 8f4bc12
|
TiDB MergeCI notify🔴 Bad News! [2] CI still failing after this pr merged.
|
What problem does this PR solve?
Issue Number: close #37651, close #37665
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.